Apply filtering to text attachments; offer to auto-upload text attachments to paste bin #3241
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ion to auto-upload to pastebin. Also DMs the delete URL to the user. This code will very likely be moved elsewhere before/if it is merged.
Works by appending text attachment content to message content, and then applying the filters normally.
swfarnsworth
added
a: moderation
Implements a somewhat arbitrary limit on how much text content is passed along for filtering, to avoid wasting compute time on large attachments that aren't intended to be read (such as CSVs)
lemonsaurus
approved these changes
Jan 29, 2025
There was a problem hiding this comment.
Tentatively approved. The feature looks solid, but I have stylistic concerns.
- Classes, methods and coroutines should have docstrings
- The cognitive complexity (e.g. mccabe) of
on_messageis too high. It's too deeply nested, too long, and doesn't break the code into logical blocks with helpful block comments headlining them as a longer piece of code ought to. I'd like to see a refactor here to move helper functions out into the class, and perhaps break up the different pieces into a few function calls with helpful names. I think generally that it's better thaton_messageitself is aggressively short and very easy to read and understand, while the implementation details are abstracted away into other functions. - Somewhat inconsistent adherence to PEP257. Comments should start with capital letters and end with punctuation, and should be imperative mood.
Looks like a great addition to the bot, though <3 thank you for writing it! I can't wait to see it merged.
| @@ -20,7 +28,7 @@ | |||
| f"please use a code-pasting service such as {PASTE_URL}" | |||
| ) | |||
|
|
|||
| TXT_LIKE_FILES = {".txt", ".csv", ".json"} | |||
| TXT_LIKE_FILES = {".txt", ".csv", ".json", ".py"} | |||
There was a problem hiding this comment.
If we're lifting the ban on .py, then anything adjacent feels like it might as well also be added. Maybe .js, .css, .html so that anyone using Python to build a webapp of some sort won't run into a brick wall? What about stuff like .yaml, .toml?
There was a problem hiding this comment.
I realized later that that code has no effect on which file types are allowed.
| self.bot = bot | ||
|
|
||
| @staticmethod | ||
| async def _convert_attachment(attachment: discord.Attachment) -> paste_service.PasteFile: |
There was a problem hiding this comment.
coroutines deserve ✨d o c s t r i n g s ✨
Previously, the bot might have offered to upload the attachments in a message containing only images, and then done nothing.
… paste. The expanded initial message tells the user that uploading to the paste bin is for accessibility. I hallucinated that PasteResponse objects have a delete method, which they do not.
…cts. Messages might be deleted immediately if the message or the attachment trips a filter, in which case we don't want the user to be able to upload them.
|
@lemonsaurus thanks for your feedback. I thought I was in the finishing-touches stage for this, but there's actually still more to be done. I'm pushing what I've worked on this evening for additional scrutiny/ridicule. |
These files will be made allowed. Also move `TXT_LIKE_FILES` to the other module that uses it.
…rect encoding. Attachments with "charset" in their content type are presumed to be text. The specified charset is always used to decode the text.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a work-in-progress intended to allow us to lift the embargo on text file attachments. Whether we use this functionality, or anything like it, is subject to further discussion. I'm submitting it as a draft PR to expose what I've done so far.
Allowing text file attachments is intended to remove friction in making a user's code available to question-answerers during help sessions. If a user uploads their py file, and it's deleted by the bot, the user might become confused and not follow the instructions to use the paste bin. Instructing users on how to use the paste bin can also be grating for question-answerers, especially when they would have been able to read the py file in the file embed that appears in the desktop client, which provides syntax highlighting.
However, the file embeds do not appear in the mobile version of Discord. Our filtering system also does not consider the content of text-based file attachments. This PR implements a solution for the former and the beginnings of a solution for the latter.
Offer to auto-upload text attachments to the paste bin
This functionality, which is already fully implemented, is intended to benefit mobile users who can't see the text file embeds, while upholding our promise not to copy user content to an external system (ie, our paste bin) without their permission. It consists of these steps, following a user sending a message containing text attachments:
Apply token filters to text attachments
This one is much trickier. The simplest solution I could think of (after trying a few more complicated ones) is to simply treat the content of text attachments as a continuation of the message itself, though this could make it not-immediately-obvious to moderators when a filter is triggered by an attachment rather than the true message content. We also don't want to waste compute time applying the filters to especially long text attachments (which is relatively likely for CSV and JSON attachments).
We might decide on a constant size limit for text-based attachments and modify the extension filters to consider both the file extension and the size of the file, while also not appending attachment content to the message if the file size exceeds that same constant. We might also not apply the token filter to CSVs and JSONs, since those are usually posted as part of a minimal reproducible example and are probably not intended to be read.